Skip to content

fix(messaging): remove duplicate inbox and unify messaging interface#993

Open
chinnu8055 wants to merge 7 commits intoalphaonelabs:mainfrom
chinnu8055:messages
Open

fix(messaging): remove duplicate inbox and unify messaging interface#993
chinnu8055 wants to merge 7 commits intoalphaonelabs:mainfrom
chinnu8055:messages

Conversation

@chinnu8055
Copy link
Contributor

@chinnu8055 chinnu8055 commented Mar 1, 2026

  • Unified messaging into a single dashboard flow and removed the duplicate inbox route to reduce confusion.
  • Added a global Starred Messages tab so important messages are visible outside individual chats.
  • Improved chat UX with unread/new visual indicators and better mobile/desktop responsiveness.
  • Enforced 7-day message retention with automated cleanup of expired direct messages.

Checklist

  • Did you run the pre-commit? (If not, your PR will most likely not pass — please ensure it passes pre-commit)
  • Did you test the change? (Ensure you didn’t just prompt the AI and blindly commit — test the code and confirm it works)
  • Added screenshots to the PR description (if applicable)
Screenshot 2026-03-01 213656 Screenshot 2026-03-01 213638

Summary by CodeRabbit

  • New Features

    • Two-pane messaging dashboard (Inbox/Starred), compose modal, mobile chat view, starring, unread badges, mark-as-read, and richer chat UI
    • End-to-end envelope-style encryption for sending and viewing messages
    • Daily background cleanup that removes messages older than 7 days
  • Security Improvements

    • Robust SVG and content sanitization for avatars and message content
  • Bug Fixes

    • Messaging header now links to the new dashboard
  • Tests

    • Added test verifying deletion of messages older than 7 days
  • Chores

    • Added defusedxml dependency

Removed /secure/inbox to eliminate the duplicate messaging flow.
Unified messaging under /messaging/dashboard with a chat-style layout.
Added a separate Starred Messages tab for quick access.
Added visual indicators for new/unread messages.
Improved responsive behavior across desktop and mobile views.
Implemented 7-day message storage retention with cleanup of expired messages.
@github-actions github-actions bot added the files-changed: 11 PR changes 11 files label Mar 1, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2026

👀 Peer Review Required

Hi @chinnu8055! This pull request does not yet have a peer review.

Before this PR can be merged, please request a review from one of your peers:

  • Go to the PR page and click "Reviewers" on the right sidebar.
  • Select a team member or contributor to review your changes.
  • Once they approve, this reminder will be automatically removed.

Thank you for contributing! 🎉

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 1, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds defusedxml dependency; implements envelope-style message encryption/decryption and SVG sanitization; introduces automated deletion of PeerMessage records older than 7 days (management command + run_daily invocation); adds mark-read endpoint; and replaces the inbox with a two-pane, client-driven messaging dashboard with starring and compose modal.

Changes

Cohort / File(s) Summary
Dependency
pyproject.toml
Added defusedxml (^0.7.1) for safer XML/SVG handling.
Daily Maintenance
web/management/commands/cleanup_expired_messages.py, web/management/commands/run_daily.py
New management command to delete PeerMessage records older than retention cutoff; run_daily now invokes this cleanup.
Messaging Core & Encryption
web/secure_messaging.py
Added SVG sanitization, message retention helpers (cutoff & cleanup), envelope encryption helpers (encrypt/decrypt with random key), mark-read endpoint, expanded messaging views (dashboard, compose, send_encrypted, download, toggle_star), imports and MESSAGE_RETENTION_DAYS = 7.
Templates — Dashboard
web/templates/web/messaging/dashboard.html
Replaced inbox with a two-pane, tabbed dashboard: people list with avatars/unread badges, compose modal, client-side chatData rendering, star toggles, responsive/mobile behavior, and read-marking fetch calls.
Templates — Legacy Messaging
web/templates/web/peer/inbox.html, web/templates/web/peer/messages.html
Removed per-message list/actions from inbox template; removed sender line inside message bubbles.
Base Template
web/templates/base.html
Updated header messaging link to point to the new messaging dashboard.
URLs
web/urls.py
Removed legacy /secure/inbox/ route; added messaging/mark_read/ endpoint.
Tests
web/tests/test_securemessaging.py
Adjusted test sender usage and added test to assert messages older than seven days are deleted.
CI Workflow
.github/workflows/codeql.yml
Set explicit source-root: ${{ github.workspace }} in CodeQL initialize step.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant Browser
    participant Server as Django Server
    participant Crypto as Encryption Module
    participant DB as Database

    Browser->>Server: GET /messaging/ (open dashboard)
    Server->>DB: Query PeerMessages for user
    Server->>Crypto: Decrypt message envelopes
    Crypto-->>Server: Decrypted message bodies
    Server->>Server: Sanitize SVG avatars (defusedxml/bleach)
    Server->>DB: Delete PeerMessages older than retention cutoff
    Server-->>Browser: Render dashboard HTML with people & chats

    Browser->>Server: POST /messaging/send (compose)
    Server->>Crypto: Encrypt message with random key (envelope)
    Crypto-->>Server: Encrypted payload + encrypted key
    Server->>DB: Store PeerMessage (encrypted)
    Server-->>Browser: Return success / message id

    Browser->>Server: POST /messaging/mark_read/
    Server->>DB: Update PeerMessage read flags for given sender/user
    Server-->>Browser: JSON success
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: removing a duplicate inbox route and unifying the messaging interface into a single dashboard.
Docstring Coverage ✅ Passed Docstring coverage is 82.35% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@web/management/commands/cleanup_expired_messages.py`:
- Line 12: The handle function in cleanup_expired_messages.py lacks type hints
and a docstring; add Python typing and documentation by annotating def
handle(self, *args: Any, **options: Any) -> None (import Any from typing) and
add a concise docstring at the top of the function describing its purpose (e.g.,
"Execute the management command to remove expired messages"), the expected
args/options usage, and return behavior; ensure imports (typing.Any) are present
and follow the repo's docstring style.

In `@web/secure_messaging.py`:
- Around line 192-197: The 7-day retention cutoff is duplicated between
cleanup_expired_peer_messages and
web/management/commands/cleanup_expired_messages.py which risks drift; extract
the retention value and cutoff calculation into a single shared symbol (e.g.,
MESSAGE_RETENTION_DAYS constant and/or a get_message_retention_cutoff() helper)
in a common module (settings or a new web/retention.py) and refactor
cleanup_expired_peer_messages and the management command to import and use that
shared constant/helper (reference: function cleanup_expired_peer_messages and
the management command in web/management/commands/cleanup_expired_messages.py).
- Around line 179-189: The view currently calls json.loads(request.body) without
handling malformed JSON and uses
PeerMessage.objects.filter(...).update(is_read=True) which bypasses save() and
leaves read_at unset; wrap the JSON parsing in a try/except for
json.JSONDecodeError and return a 400 JSON response on failure, and when marking
messages read set the timestamp as well (e.g. use
PeerMessage.objects.filter(sender=sender, receiver=request.user,
is_read=False).update(is_read=True, read_at=timezone.now()) or iterate and call
save() if per-object logic is needed); ensure you import json and
django.utils.timezone.now and keep the existing User lookup (get_user_model /
User.objects.get) and error responses.

In `@web/templates/web/messaging/dashboard.html`:
- Around line 131-137: Server-rendered usernames are inserted into JS without
escaping which can break scripts or allow injection; update the template so
chatData["{{ person.username }}"], the sender field "{{ msg.sender }}", and the
occurrences of request.user.username (lines referenced around 387–389) use the
Django escapejs filter (e.g., "{{ person.username|escapejs }}", "{{
msg.sender|escapejs }}", "{{ request.user.username|escapejs }}") so the values
are safely escaped for embedding in JavaScript string literals and bracket keys.
- Around line 28-29: The template adds custom CSS classes (person-btn,
person-unread-badge, message-star-btn) which violate the "Never use custom CSS
classes" rule — replace those class hooks with data-* attributes (e.g., change
class="... person-btn" to remove person-btn and add data-person-btn, replace
person-unread-badge with data-person-unread-badge, and message-star-btn with
data-message-star-btn) and update any JS/CSS that references these classes to
use the new data-* selectors (search for usages of person-btn,
person-unread-badge, message-star-btn and update them to
querySelector/querySelectorAll('[data-person-btn]') etc.); apply the same
replacements for the other occurrences noted in the template (lines mentioned in
the review).

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e694ce8 and 470e9f4.

⛔ Files ignored due to path filters (1)
  • poetry.lock is excluded by !**/*.lock
📒 Files selected for processing (10)
  • pyproject.toml
  • web/management/commands/cleanup_expired_messages.py
  • web/management/commands/run_daily.py
  • web/secure_messaging.py
  • web/templates/base.html
  • web/templates/web/messaging/dashboard.html
  • web/templates/web/peer/inbox.html
  • web/templates/web/peer/messages.html
  • web/tests/test_securemessaging.py
  • web/urls.py
💤 Files with no reviewable changes (2)
  • web/templates/web/peer/messages.html
  • web/templates/web/peer/inbox.html

Comment on lines +192 to +197
def cleanup_expired_peer_messages() -> int:
"""Delete direct messages older than 7 days and return deleted count."""
cutoff = timezone.now() - timedelta(days=7)
expired_qs = PeerMessage.objects.filter(created_at__lt=cutoff)
deleted_count = expired_qs.delete()[0]
return deleted_count
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Centralize retention logic to avoid drift between cleanup paths.

The 7-day cutoff logic now exists here and in web/management/commands/cleanup_expired_messages.py; keeping a single shared helper/constant would prevent future inconsistencies.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/secure_messaging.py` around lines 192 - 197, The 7-day retention cutoff
is duplicated between cleanup_expired_peer_messages and
web/management/commands/cleanup_expired_messages.py which risks drift; extract
the retention value and cutoff calculation into a single shared symbol (e.g.,
MESSAGE_RETENTION_DAYS constant and/or a get_message_retention_cutoff() helper)
in a common module (settings or a new web/retention.py) and refactor
cleanup_expired_peer_messages and the management command to import and use that
shared constant/helper (reference: function cleanup_expired_peer_messages and
the management command in web/management/commands/cleanup_expired_messages.py).

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2026

💬 Unresolved Review Conversations

Hi @chinnu8055! 👋

This pull request currently has 5 unresolved review conversations.

Please address all review feedback and push a new commit to resolve them before this PR can be merged.

Steps to resolve:

  1. Review each comment thread in the "Files changed" tab.
  2. Make the necessary changes to your code.
  3. Reply to each conversation to explain your changes or ask for clarification.
  4. Click "Resolve conversation" once the feedback has been addressed.
  5. Push a new commit with your changes.

Once all conversations are resolved, this notice will be removed automatically. Thank you! 🙏

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR has 1 unresolved review conversation. Please resolve them before this PR can be merged.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@web/management/commands/cleanup_expired_messages.py`:
- Around line 23-24: The code does an extra COUNT() by calling
expired_qs.count() before expired_qs.delete(); remove the separate count() and
use delete()'s return value instead—call expired_qs.delete(), capture its first
element (the number of rows deleted) and use that for reporting; update any
references to the old count variable to use the delete() result and drop the
now-unused expired_qs.count() call.

In `@web/secure_messaging.py`:
- Around line 308-350: The loop over user_ids causes N+1 queries (per-user User
lookup, avatar/profile access, per-conversation PeerMessage query, and unread
check). Fix by preloading data: fetch all User objects for user_ids in one query
(use User.objects.filter(id__in=...).select_related('profile') to get
profiles/avatars and avoid per-user profile hits), load all relevant PeerMessage
rows in a single query (PeerMessage.objects.filter(models.Q(sender=request.user,
receiver__in=user_ids) | models.Q(receiver=request.user,
sender__in=user_ids)).order_by('created_at') ) and group them in-memory by the
other party to build msg_list and compute has_unread counts (use annotations
like .annotate(unread=Count('id', filter=Q(receiver=request.user,
is_read=False))) or aggregate unread per sender in one query) and then iterate
over the in-memory maps to call sanitize_svg and decrypt_message_with_random_key
without additional DB calls.

In `@web/templates/web/messaging/dashboard.html`:
- Line 30: The data-person HTML attribute is using escapejs which produces
literal escape sequences and breaks lookups against the chatData keys; remove
the escapejs filter from the data-person attribute (i.e., render person.username
unescaped for HTML attribute context) so that getAttribute('data-person')
returns the real username that matches the chatData keys used in the JavaScript
(chatData and the code that builds/looks up entries).

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 470e9f4 and 9caad8c.

📒 Files selected for processing (3)
  • web/management/commands/cleanup_expired_messages.py
  • web/secure_messaging.py
  • web/templates/web/messaging/dashboard.html

from django.contrib.auth import get_user_model

User = get_user_model()
cleanup_expired_peer_messages()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Avoid running full retention cleanup in the request path.

Line 286 performs deletion work on every dashboard load. This can become a latency hotspot and increase lock contention under load. Since daily cleanup already exists, keep destructive cleanup in maintenance jobs and apply cutoff filtering in read queries for immediate UX consistency.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@web/secure_messaging.py`:
- Around line 358-362: The decryption except block swallows errors; change it to
capture the exception (except Exception as e) and log it (e.g., logger.exception
or logger.error with the exception) together with contextual identifiers from
the message (such as msg.id or msg.message_id and msg.encrypted_key) before
setting decrypted_message = "[Error decrypting message]"; use the existing
module logger (or create one) and prefer logger.exception to include the
traceback for decrypt_message_with_random_key failures.

In `@web/templates/web/messaging/dashboard.html`:
- Around line 309-317: The avatar fallback inserts trimmed server-rendered text
into innerHTML (see btn, img, fallback, chatPersonAvatar), so add
defense-in-depth by escaping the fallback string before embedding it: use or
implement an escapeHTML utility and replace fallback.textContent.trim() with
escapeHTML(fallback.textContent.trim()) (or set chatPersonAvatar.textContent for
the fallback branch) to ensure any future user-controlled data is safely
encoded.
- Around line 147-164: The openNewChatUI function uses
chatPersonAvatar.innerHTML with username.charAt(0) which can introduce XSS;
sanitize the username before inserting into the DOM by using the existing
escapeHTML utility and inject the escaped single-character avatar content (or
better, set textContent on a created element instead of raw innerHTML). Update
openNewChatUI to call escapeHTML(username) (or escapeHTML(username.charAt(0)))
and use that escaped value when populating chatPersonAvatar, keeping
chatPersonName.textContent and chat-recipient.value logic unchanged.

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9caad8c and 44075fa.

📒 Files selected for processing (3)
  • web/management/commands/cleanup_expired_messages.py
  • web/secure_messaging.py
  • web/templates/web/messaging/dashboard.html

Comment on lines +358 to +362
for msg in msgs:
try:
decrypted_message = decrypt_message_with_random_key(msg.content, msg.encrypted_key)
except Exception:
decrypted_message = "[Error decrypting message]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider logging decryption failures for debugging.

While catching a broad Exception is reasonable here since decryption can fail in various ways, logging the error would help diagnose issues with corrupted messages or key problems.

♻️ Optional improvement
+import logging
+
+logger = logging.getLogger(__name__)
+
 # In the loop:
             try:
                 decrypted_message = decrypt_message_with_random_key(msg.content, msg.encrypted_key)
             except Exception:
+                logger.exception("Failed to decrypt message id=%s", msg.id)
                 decrypted_message = "[Error decrypting message]"
🧰 Tools
🪛 Ruff (0.15.2)

[warning] 361-361: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/secure_messaging.py` around lines 358 - 362, The decryption except block
swallows errors; change it to capture the exception (except Exception as e) and
log it (e.g., logger.exception or logger.error with the exception) together with
contextual identifiers from the message (such as msg.id or msg.message_id and
msg.encrypted_key) before setting decrypted_message = "[Error decrypting
message]"; use the existing module logger (or create one) and prefer
logger.exception to include the traceback for decrypt_message_with_random_key
failures.

Comment on lines +147 to +164
function openNewChatUI(username) {
// Set chat person name and avatar (generic)
const chatPersonName = document.getElementById('chat-person-name');
const chatPersonAvatar = document.getElementById('chat-person-avatar');
chatPersonName.textContent = username;
chatPersonAvatar.innerHTML = `<div class="w-8 h-8 rounded-full bg-orange-500 flex items-center justify-center text-white font-bold border-2 border-teal-500">${username.charAt(0).toUpperCase()}</div>`;
// Set recipient for form POST
document.getElementById('chat-recipient').value = username;
// Show blank chat history
renderChatHistory(username);
// Show chat form
document.getElementById('chat-form').classList.remove('hidden');
showMobileChatView();
// Scroll to chat area
document.getElementById('chat-history').scrollIntoView({
behavior: 'smooth'
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Potential XSS via innerHTML with user-provided username.

The openNewChatUI function at line 152 uses innerHTML with username.charAt(0), which is derived from user input in the compose modal. While charAt(0) limits the risk, a malicious character like < could still cause issues. Use the existing escapeHTML function for consistency.

🛡️ Proposed fix
 function openNewChatUI(username) {
     // Set chat person name and avatar (generic)
     const chatPersonName = document.getElementById('chat-person-name');
     const chatPersonAvatar = document.getElementById('chat-person-avatar');
     chatPersonName.textContent = username;
-    chatPersonAvatar.innerHTML = `<div class="w-8 h-8 rounded-full bg-orange-500 flex items-center justify-center text-white font-bold border-2 border-teal-500">${username.charAt(0).toUpperCase()}</div>`;
+    chatPersonAvatar.innerHTML = `<div class="w-8 h-8 rounded-full bg-orange-500 flex items-center justify-center text-white font-bold border-2 border-teal-500">${escapeHTML(username.charAt(0).toUpperCase())}</div>`;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/templates/web/messaging/dashboard.html` around lines 147 - 164, The
openNewChatUI function uses chatPersonAvatar.innerHTML with username.charAt(0)
which can introduce XSS; sanitize the username before inserting into the DOM by
using the existing escapeHTML utility and inject the escaped single-character
avatar content (or better, set textContent on a created element instead of raw
innerHTML). Update openNewChatUI to call escapeHTML(username) (or
escapeHTML(username.charAt(0))) and use that escaped value when populating
chatPersonAvatar, keeping chatPersonName.textContent and chat-recipient.value
logic unchanged.

Comment on lines +309 to +317
const img = btn.querySelector('img');
const fallback = btn.querySelector('div.w-8.h-8');
if (img) {
chatPersonAvatar.innerHTML = `<img src="${img.src}" alt="Avatar" class="w-8 h-8 rounded-full object-cover border-2 border-teal-500" />`;
} else if (fallback) {
chatPersonAvatar.innerHTML = `<div class="w-8 h-8 rounded-full bg-orange-500 flex items-center justify-center text-white font-bold border-2 border-teal-500">${fallback.textContent.trim()}</div>`;
} else {
chatPersonAvatar.innerHTML = '';
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider escaping avatar fallback content for defense-in-depth.

While fallback.textContent.trim() originates from server-rendered content, using escapeHTML would add a safety layer against potential future changes where user-controlled data might flow through this path.

♻️ Optional improvement
              if (img) {
-                 chatPersonAvatar.innerHTML = `<img src="${img.src}" alt="Avatar" class="w-8 h-8 rounded-full object-cover border-2 border-teal-500" />`;
+                 chatPersonAvatar.innerHTML = `<img src="${escapeHTML(img.src)}" alt="Avatar" class="w-8 h-8 rounded-full object-cover border-2 border-teal-500" />`;
              } else if (fallback) {
-                 chatPersonAvatar.innerHTML = `<div class="w-8 h-8 rounded-full bg-orange-500 flex items-center justify-center text-white font-bold border-2 border-teal-500">${fallback.textContent.trim()}</div>`;
+                 chatPersonAvatar.innerHTML = `<div class="w-8 h-8 rounded-full bg-orange-500 flex items-center justify-center text-white font-bold border-2 border-teal-500">${escapeHTML(fallback.textContent.trim())}</div>`;
              } else {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@web/templates/web/messaging/dashboard.html` around lines 309 - 317, The
avatar fallback inserts trimmed server-rendered text into innerHTML (see btn,
img, fallback, chatPersonAvatar), so add defense-in-depth by escaping the
fallback string before embedding it: use or implement an escapeHTML utility and
replace fallback.textContent.trim() with escapeHTML(fallback.textContent.trim())
(or set chatPersonAvatar.textContent for the fallback branch) to ensure any
future user-controlled data is safely encoded.

@github-actions github-actions bot added files-changed: 12 PR changes 12 files and removed files-changed: 11 PR changes 11 files labels Mar 2, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/codeql.yml:
- Line 76: The workflow uses an absolute path for the github/codeql-action/init
input `source-root` (currently set to `${{ github.workspace }}`); change
`source-root` to a relative value such as `.` or remove the `source-root` key
entirely so it relies on the default (relative to $GITHUB_WORKSPACE) when
invoking the `github/codeql-action/init` step.

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cba809f and 2ae2c24.

📒 Files selected for processing (1)
  • .github/workflows/codeql.yml

with:
languages: ${{ matrix.language }}
build-mode: ${{ matrix.build-mode }}
source-root: ${{ github.workspace }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# First, find and display the codeql.yml file around line 76
echo "=== Current workflow file content (lines 70-85) ==="
fd -t f 'codeql\.yml$' | head -1 | xargs -r sed -n '70,85p' -

# Show the full init step context
echo -e "\n=== Full init step context ==="
fd -t f 'codeql\.yml$' | head -1 | xargs -r sed -n '1,100p' - | grep -A 15 "init@v3" || echo "Pattern not found, showing raw search"

# Check exact line 76
echo -e "\n=== Exact line 76 ==="
fd -t f 'codeql\.yml$' | head -1 | xargs -r sed -n '76p' -

Repository: alphaonelabs/website

Length of output: 208


🌐 Web query:

github codeql-action init source-root parameter documentation

💡 Result:

In the CodeQL Init GitHub Action (github/codeql-action/init), the input you’re looking for is:

  • source-root: “Path of the root source code directory, relative to $GITHUB_WORKSPACE.” (Optional.) [1]

Example:

- uses: github/codeql-action/init@v4
  with:
    source-root: path/to/subdir

This is conceptually the same idea as the CodeQL CLI’s --source-root (used to point CodeQL at a source directory when it isn’t the current working directory / checkout root). [2]

Sources: [1] [2]


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Search for all workflow files
echo "=== All workflow files in repository ==="
find . -path '*/.github/workflows/*.yml' -o -path '*/.github/workflows/*.yaml' 2>/dev/null | head -20

# Search for any file containing codeql
echo -e "\n=== Files containing 'codeql' ==="
rg -l 'codeql' --type yaml --type yml 2>/dev/null | head -20

# Search for source-root in workflow files
echo -e "\n=== Files containing 'source-root' ==="
rg 'source-root' --type yaml --type yml -n 2>/dev/null

# List the .github/workflows directory if it exists
echo -e "\n=== Contents of .github/workflows directory ==="
ls -la .github/workflows/ 2>/dev/null || echo "Directory not found"

Repository: alphaonelabs/website

Length of output: 851


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Display the full codeql.yml file
echo "=== Full codeql.yml file ==="
cat -n .github/workflows/codeql.yml

# Specifically search for source-root in this file
echo -e "\n=== Search for source-root in codeql.yml ==="
rg 'source-root' .github/workflows/codeql.yml -n || echo "No 'source-root' found"

Repository: alphaonelabs/website

Length of output: 5696


🌐 Web query:

github codeql-action init v3 source-root absolute path behavior documentation

💡 Result:

For github/codeql-action/init@v3, the documentation for the source-root input says it should be a path relative to $GITHUB_WORKSPACE (“Path of the root source code directory, relative to $GITHUB_WORKSPACE.”) [1].

However, the actual v3 implementation sets sourceRoot like this:

  • sourceRoot = path.resolve(process.env.GITHUB_WORKSPACE, (input "source-root") || "")

…and explicitly comments that path.resolve() is relied on so that:

  • if source-root is relative, it’s treated as relative to $GITHUB_WORKSPACE
  • if source-root is absolute, it is used as given [2]

So in practice, absolute paths are accepted and will override $GITHUB_WORKSPACE, even though the input description implies a workspace-relative path [1], [2].

Sources: [1] [2]


🌐 Web query:

CodeQL action source-root relative path $GITHUB_WORKSPACE absolute path

💡 Result:

codeql-action/init’s source-root must be a path relative to $GITHUB_WORKSPACE (not an absolute path). [1]

  • $GITHUB_WORKSPACE (and ${{ github.workspace }}) is the runner’s absolute workspace directory where your repo is checked out (example: /home/runner/work/<repo>/<repo>; in container jobs it maps under /github/workspace). [2]

What to put in source-root

If your code lives in a subfolder of the repo checkout, set:

- uses: github/codeql-action/init@v3
  with:
    source-root: subdir/path   # relative to $GITHUB_WORKSPACE

If you currently have an absolute path

Convert it to a workspace-relative path (i.e., strip the ${{ github.workspace }}/ prefix) because source-root is defined as relative to $GITHUB_WORKSPACE. [1]

Sources: [1] [2]


Use a relative source-root value on line 76.

The source-root input for github/codeql-action/init is documented as "relative to $GITHUB_WORKSPACE", but the current value ${{ github.workspace }} is an absolute path. While path.resolve() technically accepts absolute paths, using one contradicts the specification and is redundant (the absolute workspace path is already the default scan scope). Use source-root: . instead or remove the field entirely to rely on default behavior.

Proposed fix
-        source-root: ${{ github.workspace }}
+        source-root: .
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
source-root: ${{ github.workspace }}
source-root: .
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/codeql.yml at line 76, The workflow uses an absolute path
for the github/codeql-action/init input `source-root` (currently set to `${{
github.workspace }}`); change `source-root` to a relative value such as `.` or
remove the `source-root` key entirely so it relies on the default (relative to
$GITHUB_WORKSPACE) when invoking the `github/codeql-action/init` step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

files-changed: 12 PR changes 12 files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant